Codecov Report

❌ Patch coverage is 87.09677% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.0470%. Comparing base (b67fc73) to head (dad120c).
⚠️ Report is 22 commits behind head on master.

@@                Coverage Diff                 @@
##               master      #2755        +/-   ##
==================================================
+ Coverage     69.9742%   70.0470%   +0.0728%     
- Complexity      35502      35632       +130     
==================================================
  Files            1557       1559         +2     
  Lines          131667     131830       +163     
  Branches        24166      24205        +39     
==================================================
+ Hits            92133      92343       +210     
+ Misses          31193      31136        -57     
- Partials         8341       8351        +10     

JMH summary — classic (commit dc71340)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

_{Runner calibration (this run vs baseline hardware): bench 0.98× (26 rulers) · core-ag 1.13× (3 rulers) · core-hz 0.94× (3 rulers) · grails-ad 0.98× (3 rulers) · grails-ez 1.11× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/classic/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

JMH summary — indy (commit dc71340)

Speedup vs trailing 90-day baseline on gh-pages. Higher = faster.
1.00 = in line with history. Per-benchmark ratio, geomean within group.
Time-per-op units inverted so direction is consistent. The calibrated
column divides out this runner's speed vs the baseline hardware, as
measured by Groovy-independent pure-Java ruler benchmarks.

⚠️ Runner speed differs ≥15% from the historical baseline hardware for: grails-ad. Raw speedups are not meaningful for those parts — use the calibrated column.

_{Runner calibration (this run vs baseline hardware): bench 0.94× (26 rulers) · core-ag 1.14× (3 rulers) · core-hz 1.09× (3 rulers) · grails-ad 1.27× (3 rulers) · grails-ez 0.98× (3 rulers)}

_{Baseline: dev/bench/jmh/<part>/indy/data.js on gh-pages, trailing 90 days. Daily dashboard · Per-suite raw data}

@blackdrag Thanks for the careful review — those points were well taken. Here is how the current design addresses them.

Test-only API on HiddenClassDefiner

Agreed. The extra helpers that existed only for tests (findConstructor, strict multi-option define* overloads, etc.) have been removed. Production surface is now essentially:

• isEnabled() / the disable property
• tryDefineNestmate(Lookup, bytes, initialize) — preferred
• tryDefineNestmate(Class, bytes, initialize) — best-effort for foreign hosts only

Duplication with ProxyGeneratorAdapter / ReflectorLoader

Also agreed that define policy should not be reimplemented at each call site. Class definition (package alignment, NESTMATE + weak lifecycle, soft-fail → null) lives only in HiddenClassDefiner.

• ReflectorLoader / ClassLoaderForClassArtifacts are thin: one tryDefineNestmate + visible defineClass fallback.
• Proxy-specific policy (when a proxy may be hidden, which host to try, MockFor-style “must stay nameable” aggregates) was pulled out of ProxyGeneratorAdapter into package-private ProxyClassDefiner, so the adapter stays focused on bytecode generation and no longer reimplements the define path.

MethodHandles.lookup() and private module access

Your reading of the javadoc matches how we now structure the API.

A Lookup only carries full privilege for the class that actually called MethodHandles.lookup(). Capturing a lookup inside HiddenClassDefiner (or any other utility) cannot magically grant full access into arbitrary hosts such as java.lang.String. Relying on that for private/module access would be the wrong model.

So the intended split is:

1. Preferred: tryDefineNestmate(Lookup, …) where the nest-host class owns the lookup, e.g.
   private static final Lookup LOOKUP = MethodHandles.lookup();
   on ReflectorLoader / ProxyGeneratorAdapter. That is the correct caller-sensitive pattern.

2. Best-effort only: tryDefineNestmate(Class, …) for foreign hosts (user classes we do not control). It uses privateLookupIn from HiddenClassDefiner and is documented to succeed only when the host package is accessible to Groovy’s module (typical for unnamed application classes; not a general substitute for a host-created Lookup). Callers must always handle null and fall back to ClassLoader.defineClass.

We no longer expose a public HiddenClassDefiner.privateLookupIn that would suggest “full access for any class from a utility.” That intent and usability are no longer conflated.

Happy to adjust naming or docs further if anything still feels misleading.

I haven't done a proper review yet, but as part of some other work, I assessed whether the PR impacts potential GraalVM support if we try harder to support that in the future. It came back with below, I'm not sure we want to do what is says yet - but just wanted to capture it somewhere for now:

Two native-image observations from exercising this branch alongside the packed-closure work (GROOVY-12227). Both are small; the first is a genuine easy win.

1. The soft-fail contract doesn't hold under native image

tryDefineNestmate catches IllegalAccessException | SecurityException | LinkageError (plus IllegalArgumentException and IndexOutOfBoundsException), and the class javadoc documents the intent as returning null on the expected failure modes "so call sites fall back to ClassLoader#defineClass with one null check".

GraalVM signals "this runtime cannot define classes" with com.oracle.svm.core.jdk.UnsupportedFeatureError, which extends java.lang.Error directly:

$ javap com/oracle/svm/core/jdk/UnsupportedFeatureError.class   # from lib/svm/builder/svm.jar, GraalVM CE 25.2.4
public class com.oracle.svm.core.jdk.UnsupportedFeatureError extends java.lang.Error {

It is not a LinkageError, so it escapes the catch and propagates out of the try* method — in precisely the environment where the fallback matters most.

A blanket catch (Throwable) would contradict the javadoc's "unexpected failures ... are not swallowed as a blanket RuntimeException", so something targeted is probably wanted, e.g.:

} catch (Error e) {
    // GraalVM native image: runtime class definition is unsupported. Name-checked
    // to avoid a build-time dependency on org.graalvm.
    if ("com.oracle.svm.core.jdk.UnsupportedFeatureError".equals(e.getClass().getName())) {
        return null;
    }
    throw e;
}

2. The kill switch is baked in at build time

public static final boolean HIDDEN_CLASSES_DISABLED =
        SystemUtil.getBooleanSafe(PROPERTY_DISABLE, false);

static final, documented as "evaluated once at class-init so hot paths pay no property-lookup cost". Under native image this class is very likely initialized at build time, so the value captured is the build JVM's, and -Dgroovy.hidden.classes.disable=true at run time silently does nothing — the one escape hatch a native user would reach for.

This is the same trap I hit in GROOVY-12227: I ended up evaluating the equivalent check per link rather than caching it in a static, because a build-time-initialized class bakes in the wrong answer (the image-code property reports buildtime there, not runtime).

Possibly one fix for both

If isEnabled() did a per-call check that also returned false when
"runtime".equals(System.getProperty("org.graalvm.nativeimage.imagecode")), then the native path would never attempt the definition at all, the kill switch would work at run time, and (1) becomes belt-and-braces rather than load-bearing. If the hot-path cost of the property read is the concern, a non-final holder initialised on first use rather than at class-init keeps both properties.

Caveat on incidence

I have not observed (1) fire in practice — the coercion I tested ([run: { ... }] as Runnable) goes through java.lang.reflect.Proxy and works natively on both master and this branch, so it does not reach HiddenClassDefiner. This is from reading the code plus confirming the class hierarchy, not from a reproduced failure. Worth a targeted test if you think the proxy/reflector paths are reachable in a native image.

@paulk-asert Thanks for capturing this — both points are real and we’ve addressed them in the current code.

1. Soft-fail under native image

Agreed that UnsupportedFeatureError extends Error directly and was not covered by LinkageError.

tryDefineNestmate now soft-fails that case by class name (com.oracle.svm.core.jdk.UnsupportedFeatureError) with no GraalVM compile dependency, and rethrows other Errors. The shared path is softFailOrRethrow(Error).

2. Kill switch baked in at build time

Agreed. The old static final HIDDEN_CLASSES_DISABLED snapshot was unsafe if the class is initialized at native-image build time.

We removed that field. isEnabled() is evaluated on each call (class definition is not a hot path):

• returns false when org.graalvm.nativeimage.imagecode=runtime (skip define entirely);
• otherwise honours -Dgroovy.hidden.classes.disable=true at run time.

So native users get a working escape hatch, and the soft-fail on UnsupportedFeatureError is belt-and-braces if something still attempts define.

Happy to adjust further if you hit a concrete native path that still reaches HiddenClassDefiner unexpectedly.

Quality Gate passed

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

✅ All tests passed ✅

🏷️ Commit: dad120c
▶️ Tests: 108854 executed
⚪️ Checks: 31/31 completed

Learn more about TestLens at testlens.app.